Skip to content

Conversation

augusto2112
Copy link
Contributor

@augusto2112 augusto2112 commented Sep 13, 2022

C++ imported types have Builtin type metadata. Ask the external provider before checking with builder.

This also has the added benefit
of not parsing all the typeref metadata in cases we don't have metadata for a type, but the external provider can resolve it.

@augusto2112
Copy link
Contributor Author

@swift-ci smoke test

}

auto FD = TC.getBuilder().getFieldTypeInfo(TR);
if (FD == nullptr || FD->isStruct()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it were safer to just move it into the if (FD == nullptr || FD->isStruct()) condition?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would we want ExternalTypeInfo to be consulted when FD->isStruct() is true? In other words, should external lookup happen only when FD == nullptr

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are two situations where we want to ask the external provider:

  • We don't have any metadata for the type, ask the external provider.
  • We have metadata for the type and it's a builtin (builtins are structs), ask the external provider as it may have more accurate information.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in those two situations, should they be checked after TC.getBuilder().getFieldTypeInfo(TR)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

currently this change checks before finding out whether either of those two cases apply.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup! Just updated the patch :)

@augusto2112
Copy link
Contributor Author

swiftlang/llvm-project#5307

@swift-ci smoke test

@augusto2112
Copy link
Contributor Author

swiftlang/llvm-project#5307

@swift-ci smoke test macOS

}

const TypeInfo *visitAnyNominalTypeRef(const TypeRef *TR) {
auto AskExternalTypeInfoProvider = [&]() -> const TypeInfo * {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about queryExternalTypeInfoProvider ?

C/C++ imported types have Builtin type metadata. Ask the external
provider when the type info we have is builtin.
@augusto2112 augusto2112 force-pushed the ask-external-provider-first branch from 7c18cdd to b30fc55 Compare September 22, 2022 22:52
@augusto2112
Copy link
Contributor Author

swiftlang/llvm-project#5307

@swift-ci smoke test

@augusto2112
Copy link
Contributor Author

swiftlang/llvm-project#5307

@swift-ci smoke test macOS

@adrian-prantl
Copy link
Contributor

@swift-ci test

1 similar comment
@adrian-prantl
Copy link
Contributor

@swift-ci test

@adrian-prantl
Copy link
Contributor

Looks like TestSwiftFoundation crashed.

@augusto2112
Copy link
Contributor Author

@adrian-prantl it's actually TestSwiftFoundationTypeNotification.py (you have to test this together with swiftlang/llvm-project#5307). I haven't been able to reproduce this failure locally yet.

@augusto2112
Copy link
Contributor Author

swiftlang/llvm-project#5307

@swift-ci smoke test macOS

@augusto2112 augusto2112 merged commit 5141229 into swiftlang:main Oct 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants